Rewire ios accessibility bridge message channel to receive semantics generating event #56691
Conversation
| /** | ||
| * Gets the accessibility bridge created in this platform view. | ||
| */ | ||
| AccessibilityBridge* GetAccessibilityBridge() { return accessibility_bridge_.get(); } |
There was a problem hiding this comment.
These are public for testing only, i am not sure how to best test the change without exposing these
There was a problem hiding this comment.
This is fine for now; refactoring this class is on my to do list for a rainy day, but please add a note pointing out this is only for testing so we can clean it up in the refactor.
| @@ -113,35 +128,18 @@ class PlatformViewIOS final : public PlatformView { | |||
| id<NSObject> observer_ = nil; | |||
| }; | |||
|
|
|||
| /// Wrapper that guarantees we communicate clearing Accessibility | |||
| /// information to Dart. | |||
| class AccessibilityBridgeManager { | |||
There was a problem hiding this comment.
The sole purpose for this class is to call setSemanticsEnabled when a11y bridge is created. This is not needed even before the change because the a11y bridge will only be created when setSemanticsEnabled is called before the change.
I think this class is obsolete
There was a problem hiding this comment.
Agreed, yeah this is ICantBelieveItsNotUniquePtr with one additional feature.
| if (generating) { | ||
| if (!accessibility_bridge_) { | ||
| accessibility_bridge_ = std::make_unique<AccessibilityBridge>(owner_controller_, this, | ||
| platform_views_controller_); |
There was a problem hiding this comment.
Can I assume platform_views_controller_ contains the same owner_controller_.platformViewsController. From the code I think yes, but not too sure.
chunhtai
force-pushed
the
issues/158399
branch
from
03a2e16
to
3ff32ab
Compare
| @@ -36,7 +36,7 @@ namespace flutter { | |||
| * lifecycle. It's a long lived bridge owned by the `FlutterEngine` and can be attached and | |||
| * detached sequentially to multiple `FlutterViewController`s and `FlutterView`s. | |||
| */ | |||
| class PlatformViewIOS final : public PlatformView { | |||
| class PlatformViewIOS : public PlatformView { | |||
There was a problem hiding this comment.
Why was it necessary to remove the final?
| @@ -171,14 +190,7 @@ new PlatformMessageHandlerIos(task_runners.GetPlatformTaskRunner())) {} | |||
| "PlatformViewIOS has no ViewController."; | |||
| return; | |||
| } | |||
| if (enabled && !accessibility_bridge_) { | |||
There was a problem hiding this comment.
GitHub won't let me comment on the line above, but I think you're safe to delete the if (!owner_controller_) { ... } block now, since it's no longer used here.
| /** | ||
| * Handles accessibility message from accessibility channel. | ||
| */ | ||
| void handleAccessibilityMessage(__weak id message, FlutterReply reply); |
There was a problem hiding this comment.
This is a C++ class, so C++ naming applies:
| void handleAccessibilityMessage(__weak id message, FlutterReply reply); | |
| void HandleAccessibilityMessage(__weak id message, FlutterReply reply); |
Need to fix up the call sites though.
| /** | ||
| * Send accessibility message to accessibility channel. | ||
| */ | ||
| void sendAccessibilityMessage(__weak id message); |
There was a problem hiding this comment.
This is a C++ class, so C++ naming applies:
| void sendAccessibilityMessage(__weak id message); | |
| void SendAccessibilityMessage(__weak id message); |
Need to fix up the call sites.
| binaryMessenger:owner_controller.engine.binaryMessenger | ||
| codec:[FlutterStandardMessageCodec sharedInstance]]; | ||
| [accessibility_channel_ setMessageHandler:^(id message, FlutterReply reply) { | ||
| handleAccessibilityMessage(message, reply); |
There was a problem hiding this comment.
| handleAccessibilityMessage(message, reply); | |
| HandleAccessibilityMessage(message, reply); |
| } | ||
| } | ||
|
|
||
| void PlatformViewIOS::sendAccessibilityMessage(__weak id message) { |
There was a problem hiding this comment.
| void PlatformViewIOS::sendAccessibilityMessage(__weak id message) { | |
| void PlatformViewIOS::SendAccessibilityMessage(__weak id message) { |
| @@ -74,7 +65,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification, | |||
|
|
|||
| void AccessibilityBridge::AccessibilityObjectDidBecomeFocused(int32_t id) { | |||
| last_focused_semantics_object_id_ = id; | |||
| [accessibility_channel_ sendMessage:@{@"type" : @"didGainFocus", @"nodeId" : @(id)}]; | |||
| platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); | |||
There was a problem hiding this comment.
| platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); | |
| platform_view_->SendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); |
| if ([type isEqualToString:@"announce"]) { | ||
| NSString* message = annotatedEvent[@"data"][@"message"]; | ||
| ios_delegate_->PostAccessibilityNotification(UIAccessibilityAnnouncementNotification, message); | ||
| NSString* messageToAnnounce = message[@"data"][@"message"]; |
There was a problem hiding this comment.
This should be message_to_announce since we're in a C++ method. I'm not in love with it either.
https://google.github.io/styleguide/objcguide.html#objective-c
|
|
||
| namespace flutter { | ||
|
|
||
| namespace testing { |
There was a problem hiding this comment.
Since everything in here is in flutter::testing, do:
namespace flutter::testing {
| void SetNeedsReportTimings(bool value) override {}; | ||
|
|
||
| std::unique_ptr<std::vector<std::string>> ComputePlatformResolvedLocale( | ||
| const std::vector<std::string>& supported_locale_data) override { |
There was a problem hiding this comment.
Nothing to do in your patch, but I'm going to take a look at the base class. Really curious why we're handing back a std::unique_ptr<std::vector<std::string>> here rather than just a std::vector<std::string>. I doubt these arrays are ever more than 5-10 entries even in extreme cases.
| return {}; | ||
| } | ||
|
|
||
| void SendChannelUpdate(std::string name, bool listening) override {} |
There was a problem hiding this comment.
Similarly, nothing to do here, but we should almost certainly make this a const std::string& or a std::string_view in the base class.
|
|
||
| MockRuntimeDelegate delegate_; | ||
| UIDartState::Context& context_; | ||
| RuntimeController runtime_controller_; |
There was a problem hiding this comment.
Should these members be private:? If so, move CanUpdateSemantics up, since that almost certainly should be public :)
| }; | ||
|
|
||
| TEST_F(RuntimeControllerTest, CanUpdateSemantics) { | ||
| auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>(); |
There was a problem hiding this comment.
This should just be:
fml::AutoResetWaitableEvent message_latch;
|
|
||
| Settings settings = CreateSettingsForFixture(); | ||
| AddNativeCallback("SemanticsUpdate", | ||
| CREATE_NATIVE_ENTRY(nativeSemanticsUpdate)); |
There was a problem hiding this comment.
| CREATE_NATIVE_ENTRY(nativeSemanticsUpdate)); | |
| CREATE_NATIVE_ENTRY(native_semantics_update)); |
| UIDartState::Context context(task_runners); | ||
| RuntimeControllerTester tester(context); | ||
|
|
||
| auto nativeSemanticsUpdate = [&tester, |
There was a problem hiding this comment.
| auto nativeSemanticsUpdate = [&tester, | |
| auto native_semantics_update = [&tester, |
chunhtai
force-pushed
the
issues/158399
branch
from
3ff32ab
to
5a51507
Compare
|
all comment addressed |
|
The presub failures look real. Are you also waiting on another review from @cbracken ? |
|
oops, i will fix the ci. I am waiting for framework side change to be merged first. |
chunhtai
force-pushed
the
issues/158399
branch
from
5a51507
to
227703f
Compare
fixes #158399 engine ios flutter/engine#56691 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
chunhtai
force-pushed
the
issues/158399
branch
from
227703f
to
33de166
Compare
chunhtai
force-pushed
the
issues/158399
branch
from
16219ee
to
0991970
Compare
| const int __writeBufferStartCapacity = 64; | ||
|
|
||
| /// This is mirroring the standard codec from framework | ||
| class StandardMessageCodec { |
There was a problem hiding this comment.
I think at some point we need to move standard message codec to some place that is universally accessible and reduce duplication
chunhtai
force-pushed
the
issues/158399
branch
from
0991970
to
af16d0e
Compare
fixes flutter/flutter#158399
previously the only correct way to enable semantics is that ios embedding receive signal from native OS, it call SetSemanticsEnabled to shell and then to dart to enable semantics tree generation.
If for some reason framework decide to enable semantics first, e.g. through
SemanticsBinding.instance.ensureSemantics(typically due to integration test or ci that wants to test semantics), the update will be dropped in shell. Even if it later on receive signal from native OS to turn on semantics, it can't construct the complete accessibility tree in embedding because the updatesemantics sends diff update and previous updates are gone forever. It will end up in a broken state.This pr changes so that the only source of truth will be in the framework side. When framework starts generating the the semantics tree, it will send a GenertingSemanticsTreeSemanticsEvent to the embedding, and the embedding needs to prepare itself to accept semantics update after receiving the message.
This however require some refactoring on iOS embedding because it will only start listen to a11y message channel after semantics is enabled. I moved a11y channel listener logic to platform_view_ios instead.
framework change flutter/flutter#159163
I also need to do the Android part
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.